feat(config): add hostRequirements pre-flight validation#173
Conversation
✅ Deploy Preview for devsydev canceled.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces host requirements validation for dev containers. It adds core validation logic ( ChangesHost Requirements Validation Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 30 minutes and 32 seconds.Comment |
Validate cpus, memory, and storage requirements from devcontainer.json against the host system before container creation. Uses an injectable HostInfo interface for testability. Warnings are logged but do not block container startup, matching the advisory nature of the spec.
71f1297 to
d03f814
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/devcontainer/config/host_requirements_system.go (1)
13-16: ⚡ Quick win
WorkspacePathis dead code — remove it.No method reads
s.WorkspacePath;AvailableStorageBytesreceives the path as a parameter andsingle.goalways constructsSystemHostInfo{}. The field is misleading and could cause future confusion about where the workspace path is sourced.🔧 Proposed fix
-// SystemHostInfo provides real system resource information. -type SystemHostInfo struct { - WorkspacePath string -} +// SystemHostInfo provides real system resource information. +type SystemHostInfo struct{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainer/config/host_requirements_system.go` around lines 13 - 16, Remove the dead WorkspacePath field from the SystemHostInfo struct: delete the WorkspacePath declaration in SystemHostInfo and update any construction or usage sites that assume it exists (e.g., places that create SystemHostInfo{} in single.go). Verify AvailableStorageBytes continues to accept the path parameter and that no methods reference s.WorkspacePath; if any compile errors appear, remove or replace those references to use the path parameter instead. Run tests/build to confirm the struct change is safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/devcontainer/config/host_requirements_system.go`:
- Around line 26-35: This file uses Unix-only APIs (syscall.Statfs and
/proc/meminfo) and needs a build constraint; add a top-line build tag
"//go:build !windows" to host_requirements_system.go and keep the rest
unchanged, and add a new stub file host_requirements_system_windows.go that
provides Windows-safe implementations (matching the same exported types and
methods such as SystemHostInfo.AvailableStorageBytes and any other functions
referenced in this file) returning appropriate zero values and errors or
fallbacks consistent with prepareprobe_windows.go pattern so the package
compiles on Windows.
---
Nitpick comments:
In `@pkg/devcontainer/config/host_requirements_system.go`:
- Around line 13-16: Remove the dead WorkspacePath field from the SystemHostInfo
struct: delete the WorkspacePath declaration in SystemHostInfo and update any
construction or usage sites that assume it exists (e.g., places that create
SystemHostInfo{} in single.go). Verify AvailableStorageBytes continues to accept
the path parameter and that no methods reference s.WorkspacePath; if any compile
errors appear, remove or replace those references to use the path parameter
instead. Run tests/build to confirm the struct change is safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ec813e3-8139-4dd4-8333-f9f550a30dc3
📒 Files selected for processing (6)
e2e/tests/readconfiguration/readconfiguration.goe2e/tests/readconfiguration/testdata-host-requirements/.devcontainer/devcontainer.jsonpkg/devcontainer/config/host_requirements.gopkg/devcontainer/config/host_requirements_system.gopkg/devcontainer/config/host_requirements_test.gopkg/devcontainer/single.go
| func (s SystemHostInfo) AvailableStorageBytes(path string) (uint64, error) { | ||
| if path == "" { | ||
| path = "/" | ||
| } | ||
| var stat syscall.Statfs_t | ||
| if err := syscall.Statfs(path, &stat); err != nil { | ||
| return 0, fmt.Errorf("statfs %q: %w", path, err) | ||
| } | ||
| return stat.Bavail * uint64(stat.Bsize), nil //nolint:gosec // Bsize type varies by platform | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for existing build constraints in the config package
rg -n '//go:build' pkg/devcontainer/config/
# Check for any existing Windows-specific stub files
fd -e go . pkg/devcontainer/config/ --exec grep -l 'windows' {}
# Check if there are any CI matrix entries targeting Windows
rg -r 'windows' .github/ 2>/dev/null || trueRepository: devsy-org/devsy
Length of output: 1727
🏁 Script executed:
#!/bin/bash
# Check the actual file content - does it have a build constraint?
head -5 pkg/devcontainer/config/host_requirements_system.go
# Check if there's a Windows stub file for host requirements
fd -e go . pkg/devcontainer/config/ -exec grep -l 'AvailableStorageBytes\|TotalMemoryBytes' {} \;
# Check if Windows is actually a build target
rg -n '//go:build' pkg/devcontainer/config/host_requirements_system.go
# Look for windows stub variant
ls -la pkg/devcontainer/config/ | grep -i hostRepository: devsy-org/devsy
Length of output: 2627
🏁 Script executed:
#!/bin/bash
# Read the complete file to check for build constraints and syscalls
cat -n pkg/devcontainer/config/host_requirements_system.go
# Check if host_requirements.go (parent file) has constraint
head -5 pkg/devcontainer/config/host_requirements.go
# Search for TotalMemoryBytes implementation to find /proc/meminfo
rg -n 'TotalMemoryBytes' pkg/devcontainer/config/Repository: devsy-org/devsy
Length of output: 2351
🏁 Script executed:
#!/bin/bash
# Check if Windows is a build target in CI
rg -n 'os.*windows|windows.*os' .github/workflows/ 2>/dev/null | head -20
# Check host_requirements.go to see how SystemHostInfo is used
cat -n pkg/devcontainer/config/host_requirements.go | head -40
# Check if there's any conditional compilation around this
rg -B5 -A5 'SystemHostInfo' pkg/devcontainer/config/Repository: devsy-org/devsy
Length of output: 3148
🏁 Script executed:
#!/bin/bash
# Check main CI workflow for OS matrix
find .github/workflows -name "*.yml" -o -name "*.yaml" | xargs rg -l 'os.*matrix\|windows' 2>/dev/null | head -5
# For each workflow, check the actual matrix
rg -A10 'strategy:' .github/workflows/*.yml 2>/dev/null | head -30
# Check if any main entry point uses SystemHostInfo unconditionally
rg -B3 -A3 'SystemHostInfo{}' pkg/Repository: devsy-org/devsy
Length of output: 1715
🏁 Script executed:
#!/bin/bash
# Check if SystemHostInfo is instantiated/used in any main packages
rg -n 'SystemHostInfo' pkg/ --type go
# Check what happens in main/entry points for Windows compatibility
rg -n 'main\|init' cmd/ --type go -A5 | head -40
# Check if there's build tag in any importing files
rg -B2 'host_requirements' pkg/ --type go | grep -E '//go:build|SystemHostInfo' | head -20Repository: devsy-org/devsy
Length of output: 659
🏁 Script executed:
#!/bin/bash
# Check the context where SystemHostInfo is used
cat -n pkg/devcontainer/single.go | sed -n '220,230p'
# Verify Windows build would fail - check if there's any conditional around the instantiation
rg -B10 -A5 'SystemHostInfo\{\}' pkg/devcontainer/single.goRepository: devsy-org/devsy
Length of output: 1069
Add build constraint to prevent Windows compilation errors — syscall.Statfs and /proc/meminfo are Unix-only.
host_requirements_system.go uses Unix-specific syscalls (syscall.Statfs at line 31 and /proc/meminfo at line 38) but has no //go:build constraint. Since Windows is a supported target (present in CI workflows), this file will fail to compile on Windows. Add //go:build !windows at the top and create a host_requirements_system_windows.go stub implementation following the existing pattern used by prepareprobe.go / prepareprobe_windows.go in the same package.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/devcontainer/config/host_requirements_system.go` around lines 26 - 35,
This file uses Unix-only APIs (syscall.Statfs and /proc/meminfo) and needs a
build constraint; add a top-line build tag "//go:build !windows" to
host_requirements_system.go and keep the rest unchanged, and add a new stub file
host_requirements_system_windows.go that provides Windows-safe implementations
(matching the same exported types and methods such as
SystemHostInfo.AvailableStorageBytes and any other functions referenced in this
file) returning appropriate zero values and errors or fallbacks consistent with
prepareprobe_windows.go pattern so the package compiles on Windows.
Split syscall.Statfs usage into platform-specific files so the package compiles on Windows where syscall.Statfs_t is undefined.
1 participant
Summary
ValidateHostRequirementsinpkg/devcontainer/config/host_requirements.gothat checks CPU count, memory, and storage against host system resources before container creationHostInfointerface so tests can mock system detection; production implementation reads/proc/meminfoand callssyscall.StatfsresolveNewContainerinpkg/devcontainer/single.go— logs warnings for unmet requirements without blocking startup (advisory per spec)hostRequirementsis preserved inread-configurationoutputSummary by CodeRabbit
New Features
Tests